fix(tokens): publish token events only after commit - #2245
Conversation
Token add and delete events were published while the database transaction that produced them was still open, so a subscriber could observe a token that was later rolled back and never persisted. Buffer the events on DBTransaction and publish them from Commit, once the underlying commit succeeded, or from FlushEvents when the transaction is owned by the caller. Rollback discards them. Service.AppendValid applies a request to a transaction it does not own, so it now returns a PostCommit function that the owner of the transaction invokes once its commit succeeded. The finality listener calls it after committing and before waking the finality waiters, so a confirmed transaction is never observed with its token events still pending. Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
6791660 to
dc383fd
Compare
AkramBitar
left a comment
AkramBitar
left a comment
There was a problem hiding this comment.
The fix is real and correct — I verified it rather than taking the description at face value. On the merge-base I wired tokens.Service with fakes and called AppendValid directly: it published 2 events with CommitCallCount() == 0, so events really did escape from inside a still-open transaction. The new code buffers them and flushes after tx.Commit(), in the right order (commit → publish → notify-status).
Also checked: go build ./... (root + integration), go vet ./..., gofmt -l on the touched packages, and go test plus go test -race on ./token/services/tokens/... and ./token/services/ttx/finality/... — all clean. go generate ./... in token/services/ttx/finality reproduces mock/tokens_service.go byte-identically. AppendValid has exactly one non-test caller (finality.Commit, shared by the listener and the recovery handler) and AppendToken/DeleteToken are the only event producers, so no path is left un-flushed.
No defects in the buffering mechanism itself: Notify's Notifier == nil guard means pending can never be non-empty with a nil Notifier, so FlushEvents' unguarded Publish cannot nil-deref; Rollback clears before delegating (the safe direction if the underlying rollback fails); err at listener.go:212 is reassigned rather than shadowed, so the named-return rollback defer in AppendValid still fires on every error path; PostCommit is a type alias, so *tokens.Service still satisfies the finality tokensService interface; and double-publish is covered by FlushEvents draining the buffer.
Two minor findings below, both low severity — neither blocks merge.
| return errors.WithMessagef(err, "transaction [%s], failed to check existence in db", txID) | ||
| return noPostCommit, errors.WithMessagef(err, "transaction [%s], failed to check existence in db", txID) | ||
| } | ||
| if exists { |
There was a problem hiding this comment.
Low — worth writing down rather than changing. This fix converts token-event delivery from at-least-once to at-most-once, and the TransactionExists short-circuit makes the loss permanent. The PR description does not mention the trade-off.
Concretely: finality.Commit calls tx.Commit(); the server commits but the client sees an error (connection reset, driver timeout on the commit ack), or the process dies between tx.Commit() returning and publishTokenEvents(ctx) at listener.go:239. The tokens are durably in the store but no event was published. On the retryRunner retry — or on the next pass of the recovery handler (recovery.go:125) — AppendValid finds exists == true and returns noPostCommit, so the store-token/delete-token events for that transaction are never emitted.
For the one in-repo subscriber (certifier/interactive) this self-heals only at the next process start via Scan() (driver.go:172); a third-party subscriber loses the notification silently.
The previous behaviour was worse — events for transactions that never committed — so this is still the right direction. But it belongs in the contract section of docs/services/tokens.md, or the recovery path should re-emit events for an already-applied transaction.
There was a problem hiding this comment.
Low — test-only, but a silent coverage regression on exactly the code this PR rewrites.
Three pre-existing negative assertions became vacuous, because they assert pub.PublishCallCount() == 0 on a transaction that is never committed — and events are now buffered until commit, so they would pass no matter what the code under test does:
TestTransaction_AppendToken:64(no owners → no event)TestTransaction_AppendToken_NoNotify:276(empty owner ID → no event)TestTransaction_DeleteToken_AbsentTokenIsNotAnError:355(token absent locally → no event)
Verified by mutation: deleting the if len(id) == 0 { continue } guard in AppendToken (storage.go:235) makes the base suite fail (TestTransaction_AppendToken_NoNotify: got 1, want 0) but leaves this PR's suite fully green.
The PR correctly added tx.Commit(ctx) to the two positive tests and to TestTransaction_Notify_NoPublisher, just not to these three. require.NoError(t, tx.Commit(ctx)) before each assertion restores the property each comment claims to protect.
(Anchored at file level because these lines are outside the diff hunks.)
2 participants
Fixes #2183
AddToken/DeleteTokenevents were published from inside the still-open database transaction that produced them, so a subscriber could act on a token that was later rolled back and never persisted.The events are now buffered on
DBTransactionand published only once the change is durable:DBTransaction.Commit(ctx)publishes them, and only if the underlying commit succeeded — this covers the transactions the service owns.DBTransaction.Rollbackdiscards them.DBTransaction.FlushEvents(ctx)publishes them explicitly, for transactions owned by the caller.Service.AppendValidapplies a request to a transaction it does not own, so it now returns aPostCommitfunction (never nil, idempotent) that the owner invokes once its own commit succeeded. The finality listener calls it aftertx.Commit()and beforeNotifyStatus, so a woken finality waiter never sees a confirmed transaction whose token events are still pending.Tests cover both ownership models: nothing published before commit, nothing published on rollback or on a failed commit, events published in recording order after commit, and the listener ordering commit → publish → notify-status.
docs/services/tokens.mddocuments the new contract.